Skip to content

backport: feat(ui): add sync status panel to wallet screen#642

Merged
lklimek merged 25 commits intov1.0-devfrom
zk-extract/sync-status-panel
Feb 25, 2026
Merged

backport: feat(ui): add sync status panel to wallet screen#642
lklimek merged 25 commits intov1.0-devfrom
zk-extract/sync-status-panel

Conversation

@lklimek
Copy link
Contributor

@lklimek lklimek commented Feb 24, 2026

Summary

  • Adds a compact sync status panel between wallet selection and wallet detail panel
  • Core line shows: RPC connected/disconnected, or SPV with phase-specific progress (Headers/Filter Headers/Filters/Blocks with %)
  • Platform line shows: address count, sync block height, and relative "time ago" since last sync
  • Adds format_unix_time_ago() / format_duration_ago() time formatting helpers

Test plan

Manual test scenarios: docs/ai-design/2026-02-24-sync-status-panel/manual-test-scenarios.md

  • Panel visible when HD wallet selected, hidden otherwise
  • RPC mode: shows Connected/Disconnected
  • SPV mode: shows sync phase with progress percentage
  • Platform: shows address count, height, time-ago
  • Wallet switching updates panel
  • Light/dark mode theming

🤖 Co-authored by Claudius the Magnificent AI Agent

Summary by CodeRabbit

  • New Features

    • Sync Status Panel on Wallets screen (Core RPC/SPV + per-wallet Platform sync) with SPV progress text and time-ago timestamps (developer mode).
    • Connection status tooltip now shows richer SPV progress and unbanned-vs-total endpoint info.
  • Bug Fixes

    • Prevent stale in-memory transaction tracking on asset lock broadcast failures.
    • Replace crash-prone unwraps in signing flows with graceful error handling.
  • Documentation

    • Added comprehensive manual test scenarios for the Sync Status Panel.

lklimek and others added 2 commits February 24, 2026 10:25
…ount

Replace hardcoded 3000 duff fee with dynamic fee calculation that accounts
for actual number of inputs. Estimates tx size using standard component
sizes (P2PKH input ~148B, output ~34B, header ~10B, payload ~60B) and
uses max(3000, estimated_size) to always meet the min relay fee.

Properly handles fee shortfall when allow_take_fee_from_amount is set,
and returns clear error messages for insufficient funds.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@lklimek lklimek self-assigned this Feb 24, 2026
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 24, 2026

Warning

Rate limit exceeded

@lklimek has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 10 minutes and 28 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between cb8ceaf and 0e7fe20.

📒 Files selected for processing (4)
  • docs/ai-design/2026-02-24-sync-status-panel/manual-test-scenarios.md
  • src/backend_task/core/create_asset_lock.rs
  • src/context/connection_status.rs
  • src/ui/wallets/wallets_screen/mod.rs
📝 Walkthrough

Walkthrough

Adds a manual test plan for the Sync Status Panel; hardens asset-lock and signing code by replacing unwraps with propagated errors and cleaning up on broadcast failure; increases numeric width for a fee log diff; adjusts wallet funding/change flow to handle fee strategies and derive change earlier; refines connection/SPV status formatting; and implements an SPV-aware per-wallet platform_sync_info cache and sync-status panel (dev-mode) with time-ago rendering.

Changes

Cohort / File(s) Summary
Documentation & Tests
docs/ai-design/2026-02-24-sync-status-panel/manual-test-scenarios.md
Adds 24 numbered manual test scenarios plus edge cases covering Core/SPV states, SPV phases, time-ago formatting, wallet switching, visuals, progress, and expected color/spinner/text behavior.
Asset Lock Broadcast & Mutex Safety
src/backend_task/core/create_asset_lock.rs
Replace poisoned-mutex unwraps with fallible lock() handling; on broadcast failure remove tx_id from transactions_waiting_for_finality before returning an error and log cleanup issues.
UTXO / Sighash Error Handling
src/model/wallet/asset_lock_transaction.rs
Replace expect/unwrap with ok_or_else/map_err and propagate errors for missing UTXOs and sighash failures; collect sighashes as Result to fail gracefully and narrow borrow scopes.
Fee Logging Precision
src/backend_task/identity/top_up_identity.rs
Use i128 for fee-difference logging calculation instead of i64 to avoid overflow for large values.
Wallet Funding / Change Strategy
src/backend_task/wallet/fund_platform_address_from_wallet_utxos.rs
Reorder step numbering and derive a fresh change address earlier; add explicit handling for "fee deducted from output" vs "fee not deducted" flows, constructing outputs and applying ReduceOutput appropriately; error when change address absent.
Connection Status & SPV Helpers
src/context/connection_status.rs
Change tooltip_text signature to tooltip_text(&self, app_context: &crate::context::AppContext) -> String; add pub fn spv_phase_summary(progress: &SpvSyncProgress) -> String and helper pct(...); show SPV progress in tooltip and make DAPI endpoint availability explicit.
Top Panel Tooltip Update
src/ui/components/top_panel.rs
Call updated status.tooltip_text(app_context) to obtain context-aware tooltip text.
Wallets UI: SPV + Platform Sync Panel
src/ui/wallets/wallets_screen/mod.rs
Add platform_sync_info: Option<(u64, u64)> to WalletsBalancesScreen; cache and refresh per-wallet platform sync (timestamp, height), add time-ago helpers and render_sync_status (two-line Core + Platform) integrated into wallets screen (dev mode); refresh/clear cache on wallet select/remove, refresh, and platform-balance updates.

Sequence Diagram(s)

sequenceDiagram
    participant UI as Wallets Screen UI
    participant Cache as platform_sync_info Cache
    participant DB as Database / Storage
    participant SPV as SPV Service

    UI->>DB: request platform sync info (seed_hash / wallet id)
    DB-->>Cache: return (last_sync_timestamp, last_sync_height)
    Cache-->>UI: provide cached platform_sync_info

    UI->>SPV: query SpvStatus & SpvSyncProgress (via AppContext)
    SPV-->>UI: return status + progress

    UI->>UI: format time-ago & spv_phase_summary
    UI->>UI: render_sync_status (Core line + Platform line)

    Note over UI,DB: on wallet refresh / platform-balance update
    UI->>DB: refresh platform sync info
    DB-->>Cache: update cached values
    Cache-->>UI: re-render sync status
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related issues

Poem

🐰 I hopped through rust with nimble hops,
Swapped panics for errors, mended stops.
Time-ago whispers and SPV light,
Panel hums true in dev-mode sight.
Carrot cheers — the sync now hops just right!

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 75.76% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'backport: feat(ui): add sync status panel to wallet screen' accurately describes the primary change—adding a sync status panel UI component to the wallet screen.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch zk-extract/sync-status-panel

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

lklimek and others added 2 commits February 24, 2026 11:27
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
lklimek and others added 14 commits February 24, 2026 12:15
…d signed

Previously, `asset_lock_transaction_from_private_key` called
`take_unspent_utxos_for` which immediately removed selected UTXOs from
`Wallet.utxos`. Since fee recalculation and signing happen afterward,
any failure at those steps (fee shortfall, missing private key, change
address derivation error) would permanently drop UTXOs — especially
dangerous in SPV mode where there is no Core RPC reload fallback.

Fix:
- Add `select_unspent_utxos_for` (`&self`, non-mutating) that performs
  the same UTXO selection logic without removing anything.
- Add `remove_selected_utxos` (`&mut self`) for explicit removal.
- Refactor `take_unspent_utxos_for` to delegate to these two methods
  (no behavior change for existing callers).
- In `asset_lock_transaction_from_private_key`, use
  `select_unspent_utxos_for` for selection and only call
  `remove_selected_utxos` after the full tx is built and signed.

Co-authored-by: lklimek <842586+lklimek@users.noreply.github.com>
# Conflicts:
#	docs/ai-design/2026-02-24-asset-lock-fee-fix/manual-test-scenarios.md
#	src/model/wallet/asset_lock_transaction.rs
…ce recalc into remove_selected_utxos

Previously, every backend task caller had to manually: (1) remove UTXOs
from the in-memory map, (2) drop them from the database, and (3)
recalculate affected address balances.  This was error-prone — the
payment transaction builders were missing the balance recalculation
entirely.

Now `remove_selected_utxos` accepts an optional `&AppContext` and
handles all three steps atomically.  The redundant cleanup blocks in
5 backend task callers are removed.  Also applies the safe
select-then-commit UTXO pattern to `build_standard_payment_transaction`
and `build_multi_recipient_payment_transaction`, fixing the same
UTXO-loss-on-signing-failure bug that was previously fixed only for
asset lock transactions.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Add checked arithmetic to UTXO selection (amount + fee overflow safety)
- Replace hardcoded fee in single-UTXO path with calculate_asset_lock_fee
- Add UTXO selection retry when real fee exceeds initial estimate
- Document write-lock invariant on select_unspent_utxos_for
- Replace .unwrap() with .map_err() on wallet write locks
- Restrict Database::shared_connection visibility to pub(crate)

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…Network directly

Replace Option<&AppContext> with concrete dependencies (&Database, Network),
removing the need for take_unspent_utxos_for. Extract balance recalculation
into a private helper reused by both remove_selected_utxos and the existing
AppContext-based wrapper.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Refresh platform sync info cache after wallet refresh completes
- Add broadcast failure cleanup in create_asset_lock (remove stale
  finality tracking entries, replace mutex .unwrap() with .map_err())
- Replace .expect() with proper error propagation in signing loops
- Use i128 for fee logging subtraction to prevent overflow
- Renumber step comments sequentially after refactoring

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@lklimek lklimek marked this pull request as ready for review February 24, 2026 17:12
@lklimek lklimek requested review from Copilot February 24, 2026 17:13
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds a sync status panel to the wallet screen showing real-time Core and Platform synchronization status. The panel displays Core connection state (RPC connected/disconnected or SPV sync progress with phase-specific percentages) and Platform sync information (address count, sync height, and time since last sync). The PR also includes several bug fixes: improved error handling in asset lock transactions, more descriptive connection status text, corrected comment numbering, and a fix for integer overflow in fee calculation logging.

Changes:

  • Adds a compact sync status panel to the wallets screen with Core and Platform status lines, showing RPC/SPV connection state and platform sync details with relative timestamps
  • Improves error handling in asset lock transaction building by replacing panics with proper error propagation
  • Includes comprehensive manual test scenarios documentation covering 25+ test cases and 6 edge cases

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
src/ui/wallets/wallets_screen/mod.rs Adds sync status panel rendering, platform sync info caching, time formatting helpers, and integrates with screen lifecycle
src/model/wallet/asset_lock_transaction.rs Replaces expect() panics with proper error handling using ok_or_else() and map_err()
src/context/connection_status.rs Updates status text to be more descriptive (e.g., "Ready" instead of "SPV synced", more detailed endpoint counts)
src/backend_task/wallet/fund_platform_address_from_wallet_utxos.rs Fixes step comment numbering from 7-8 to 6-7-8
src/backend_task/identity/top_up_identity.rs Changes fee difference calculation to use i128 cast to handle negative differences
src/backend_task/core/create_asset_lock.rs Adds broadcast error handling with cleanup of finality tracking and uses map_err for mutex locks
docs/ai-design/2026-02-24-sync-status-panel/manual-test-scenarios.md Comprehensive manual test documentation with 25 test scenarios and 6 edge cases

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@lklimek
Copy link
Contributor Author

lklimek commented Feb 24, 2026

In SPV mode, I keep having Platform: Addresses: never synced. We need to investigate that.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (2)
src/backend_task/wallet/fund_platform_address_from_wallet_utxos.rs (2)

82-86: Inconsistent error handling: lock().unwrap() vs lock().map_err()?.

Line 84 still uses lock().unwrap() on transactions_waiting_for_finality, while the same operation in create_asset_lock.rs (lines 36–39, 93–96) was updated in this PR to use lock().map_err(|e| e.to_string())?. For consistency and to avoid a panic on a poisoned lock, consider aligning this call.

Proposed fix
-            let mut proofs = self.transactions_waiting_for_finality.lock().unwrap();
+            let mut proofs = self
+                .transactions_waiting_for_finality
+                .lock()
+                .map_err(|e| e.to_string())?;

Based on learnings, error handling refactoring is needed across the codebase, particularly to avoid panics with .expect() and instead propagate errors properly using the ? operator.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/backend_task/wallet/fund_platform_address_from_wallet_utxos.rs` around
lines 82 - 86, Replace the panic-prone call to
transactions_waiting_for_finality.lock().unwrap() with fallible error
propagation consistent with other modules: acquire the mutex with
transactions_waiting_for_finality.lock().map_err(|e| e.to_string())? and then
perform proofs.insert(tx_id, None); so the function (the block in
fund_platform_address_from_wallet_utxos where the "Step 2" registration occurs)
returns an Err instead of panicking when the lock is poisoned, matching the
pattern used in create_asset_lock.rs.

109-118: try_lock() in cleanup may skip cleanup unnecessarily.

Line 113 uses try_lock() which will fail not only on a poisoned lock but also if another thread momentarily holds the mutex. The analogous cleanup in create_asset_lock.rs (lines 52, 105) uses lock() which waits for the mutex. Since this is an error-recovery path, it's better to wait briefly rather than risk leaving a stale entry in the finality map.

Proposed fix
-            if let Ok(mut proofs) = self.transactions_waiting_for_finality.try_lock() {
+            if let Ok(mut proofs) = self.transactions_waiting_for_finality.lock() {
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/backend_task/wallet/fund_platform_address_from_wallet_utxos.rs` around
lines 109 - 118, The cleanup path after broadcast_raw_transaction currently uses
transactions_waiting_for_finality.try_lock(), which can fail transiently and
skip removing tx_id; change it to acquire the mutex with
transactions_waiting_for_finality.lock().await (or blocking lock() as used in
create_asset_lock.rs) so the code waits for the lock and reliably removes the
entry, then proceed to call
db.delete_asset_lock_transaction(tx_id.as_byte_array()) and return the error as
before; ensure you reference the same mutex (transactions_waiting_for_finality)
and tx_id removal logic to mirror the safe cleanup in create_asset_lock.rs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@src/backend_task/wallet/fund_platform_address_from_wallet_utxos.rs`:
- Around line 82-86: Replace the panic-prone call to
transactions_waiting_for_finality.lock().unwrap() with fallible error
propagation consistent with other modules: acquire the mutex with
transactions_waiting_for_finality.lock().map_err(|e| e.to_string())? and then
perform proofs.insert(tx_id, None); so the function (the block in
fund_platform_address_from_wallet_utxos where the "Step 2" registration occurs)
returns an Err instead of panicking when the lock is poisoned, matching the
pattern used in create_asset_lock.rs.
- Around line 109-118: The cleanup path after broadcast_raw_transaction
currently uses transactions_waiting_for_finality.try_lock(), which can fail
transiently and skip removing tx_id; change it to acquire the mutex with
transactions_waiting_for_finality.lock().await (or blocking lock() as used in
create_asset_lock.rs) so the code waits for the lock and reliably removes the
entry, then proceed to call
db.delete_asset_lock_transaction(tx_id.as_byte_array()) and return the error as
before; ensure you reference the same mutex (transactions_waiting_for_finality)
and tx_id removal logic to mirror the safe cleanup in create_asset_lock.rs.

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5a13ded and 6ffb0ee.

📒 Files selected for processing (7)
  • docs/ai-design/2026-02-24-sync-status-panel/manual-test-scenarios.md
  • src/backend_task/core/create_asset_lock.rs
  • src/backend_task/identity/top_up_identity.rs
  • src/backend_task/wallet/fund_platform_address_from_wallet_utxos.rs
  • src/context/connection_status.rs
  • src/model/wallet/asset_lock_transaction.rs
  • src/ui/wallets/wallets_screen/mod.rs

@PastaPastaPasta
Copy link
Member

Screenshot 2026-02-24 at 19 25 17 Screenshot 2026-02-24 at 19 25 35 Screenshot 2026-02-24 at 19 27 55

Concept NACK. I don't think it makes sense to show this on the wallets page; this is relevant to most DET operations.

Maybe instead we enhance the sync indicator in top left to also show progress, but I don't like this approach.

Copy link
Member

@PastaPastaPasta PastaPastaPasta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

concept NACK

lklimek and others added 2 commits February 25, 2026 09:55
The PlatformAddressBalances task result handler updated wallet balances
but did not refresh the platform_sync_info cache, causing the UI to
display "never synced" until the wallet was reselected.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The Core/Platform sync status panel is now hidden by default and only
visible when developer mode is enabled. It uses a collapsible header
so developers can expand/collapse it as needed.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/ui/wallets/wallets_screen/mod.rs`:
- Around line 179-184: The current chain that builds platform_sync_info
(starting from selected_wallet.as_ref().and_then(|w| w.read().ok().map(|g|
g.seed_hash())).and_then(|hash|
app_context.db.get_platform_sync_info(&hash).ok()).map(|(ts, checkpoint,
_terminal)| (ts, checkpoint)).filter(|(ts, _)| *ts > 0)) throws away valid
cached checkpoint data when ts == 0; remove or change the final .filter(|(ts,
_)| *ts > 0) so that platform_sync_info preserves the (ts, checkpoint) tuple
even if ts == 0 (or replace the filter with a check that only discards when both
ts == 0 and checkpoint indicates no data), and update any downstream handling
(e.g., code that renders "Addresses: never synced") to consider checkpoint
presence rather than assuming ts > 0 is required for valid sync info; locate
this logic around selected_wallet, platform_sync_info, seed_hash and
app_context.db.get_platform_sync_info to apply the change.
- Around line 120-121: The platform_sync_info cache is only updated in
select_hd_wallet, causing stale sync state when selected_wallet is reassigned
elsewhere (e.g., post-removal or fallback reselection paths); add a single
helper (e.g., refresh_platform_sync_info or set_selected_wallet) that both
assigns selected_wallet and updates platform_sync_info (using the same logic as
in select_hd_wallet) and replace all direct assignments to selected_wallet
across code paths with calls to this helper so the sync panel always reflects
the newly selected wallet.

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6ffb0ee and 0406d47.

📒 Files selected for processing (1)
  • src/ui/wallets/wallets_screen/mod.rs

@lklimek
Copy link
Contributor Author

lklimek commented Feb 25, 2026

🔍 Audit Summary — PR #642

Reviewed by: Claude Code with a 3-agent team:

  • rust-developer — Code quality, idiomatic patterns, error handling, lock safety
  • security-engineer — Security audit, OWASP classification, state consistency
  • project-reviewer — Convention adherence, cross-file consistency, test coverage

Overall risk: Low. No critical or high-severity issues. The PR improves codebase robustness (panic removal, Mutex error handling) while adding a well-gated diagnostic UI panel.


Findings

# Severity Location Description
1 🟡 Medium create_asset_lock.rs:43-56 UTXO state inconsistency on broadcast failure — Comment says "we trigger a wallet refresh" but code does not. UTXOs removed from memory but never spent on-chain → incorrect balance until manual refresh. [A04:2021]
2 🟡 Medium wallets_screen/mod.rs:1305-1347 Missing masternodes sync phase + duplicated SPV phase logicspv_active_phase_text omits the Masternodes phase (present in NetworkChooserScreen::format_sync_progress) and duplicates phase interpretation logic. Users see generic "syncing..." during masternodes phase.
3 🟡 Medium wallets_screen/mod.rs:820-843 Utility functions trapped on screen structformat_unix_time_ago/format_duration_ago are pure functions with no self dependency. Should live in a shared utility module for reuse.
4 🟡 Medium create_asset_lock.rs:52 Silent Mutex poisoning in cleanup path — Happy path propagates lock errors, but cleanup uses if let Ok(...) which silently ignores poisoned Mutex. Should at minimum log a warning. [A04:2021]
5 🟢 Low wallets_screen/mod.rs No unit tests for new helper functionsformat_duration_ago, simple_progress_pct, spv_active_phase_text are pure logic, trivially testable, but have no tests.
6 🟢 Low manual-test-scenarios.md TS-25 references work not in this PR — Describes shared_connection removal, but Database::shared_connection() still exists and the diff doesn't touch it.

Pre-existing / Outside-diff observations

# Severity Description
P1 🟡 Medium 11 remaining .unwrap() on transactions_waiting_for_finality Mutex across transaction_processing.rs, register_identity.rs, top_up_identity.rs, fund_platform_address_from_wallet_utxos.rs. Any panic there poisons the Mutex for the new graceful callers. Track as follow-up.

✅ Positive observations

  • Panic removal in signing code (asset_lock_transaction.rs) — .expect() → proper Result propagation with descriptive error messages. Excellent hardening.
  • Broadcast failure cleanup — Removing stale transactions_waiting_for_finality entries on failure prevents phantom "waiting for finality" states.
  • Dev-mode gating — Sync panel behind is_developer_mode() + collapsible. Good UX design.
  • i64i128 cast for fee diff logging prevents overflow on large credit values.
  • saturating_sub for time calculations — Prevents underflow on clock skew.
  • Division-by-zero guard in simple_progress_pct with target == 0 check.
  • Connection status tooltip improvement — "5 unbanned / 10 total" is clearer than "5/10".
  • Manual test scenarios are thorough (24 cases + edge cases table) and correctly placed per project conventions.

Redundancy analysis

3 agents produced 18 raw findings. After deduplication: 6 unique actionable findings + 1 pre-existing observation. Redundancy ratio: 39% (7 findings flagged by 2+ agents). Key overlap areas: Mutex handling strategy, SPV phase logic concerns, and time formatting edge cases.


🤖 Reviewed by Claudius the Magnificent AI Agent | rust-developer · security-engineer · project-reviewer

lklimek and others added 2 commits February 25, 2026 11:14
- Centralize wallet selection via set_selected_hd_wallet() helper to
  keep platform_sync_info cache consistent across all code paths
- Add tracing::warn! for Mutex poisoning in asset lock cleanup paths
- Fix misleading comment about wallet refresh on broadcast failure
- Remove TS-25 from manual test scenarios (not part of this PR)

Refs: #657

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Consolidate duplicated SPV sync phase formatting into a shared
`spv_phase_summary()` function in `connection_status.rs`. The wallet
screen now uses this shared function instead of its own copy. The
network screen retains its richer operator-facing formatter.

The connection indicator tooltip now shows detailed sync progress
(e.g. "SPV: Headers: 12345 / 27000 (45%)") instead of bare
"SPV: Syncing" when in SPV mode.

Also adjust refresh polling rates: 4s when connected, 1s when
disconnected (was 10s/2s).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)
src/ui/wallets/wallets_screen/mod.rs (2)

2140-2156: ⚠️ Potential issue | 🟠 Major

Guard platform sync cache refresh to the active wallet only.

At Line 2155, self.refresh_platform_sync_info_cache(&seed_hash) runs even if the async result belongs to a wallet that is no longer selected. That can make the panel show another wallet’s sync timestamp/height after wallet switching.

✅ Suggested fix
             crate::ui::BackendTaskSuccessResult::PlatformAddressBalances {
                 seed_hash,
                 balances,
             } => {
                 self.refreshing = false;
+                let is_selected_wallet = self
+                    .selected_wallet
+                    .as_ref()
+                    .and_then(|w| w.read().ok().map(|g| g.seed_hash()))
+                    .is_some_and(|selected_hash| selected_hash == seed_hash);
+
                 // Update wallet's platform_address_info if this is for the selected wallet
                 if let Some(selected) = &self.selected_wallet
                     && let Ok(mut wallet) = selected.write()
                     && wallet.seed_hash() == seed_hash
                 {
                     // Update balances in the wallet
                     for (addr, (balance, nonce)) in balances {
                         wallet.set_platform_address_info(addr, balance, nonce);
                     }
                 }
-                self.refresh_platform_sync_info_cache(&seed_hash);
+                if is_selected_wallet {
+                    self.refresh_platform_sync_info_cache(&seed_hash);
+                }
                 self.set_message(
                     "Successfully synced Platform balances".to_string(),
                     MessageType::Success,
                 );
             }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/ui/wallets/wallets_screen/mod.rs` around lines 2140 - 2156, The
PlatformAddressBalances branch updates the selected wallet but always calls
self.refresh_platform_sync_info_cache(&seed_hash) even if the result no longer
corresponds to the currently selected wallet; modify the
BackendTaskSuccessResult::PlatformAddressBalances handling so that after
verifying the selected wallet (the selected_wallet read lock and
wallet.seed_hash() == seed_hash) and updating balances via
wallet.set_platform_address_info(...) you only call
self.refresh_platform_sync_info_cache(&seed_hash) when that same selected wallet
check succeeded (i.e., wrap the refresh call inside the same if-let guard or
re-check selected_wallet and seed_hash before calling
refresh_platform_sync_info_cache).

12-27: ⚠️ Potential issue | 🟡 Minor

Import block needs rustfmt cleanup to unblock CI.

The pipeline is currently failing on formatting/import ordering for this file; please run rustfmt on this module.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/ui/wallets/wallets_screen/mod.rs` around lines 12 - 27, The import block
in this module is misformatted; run rustfmt (cargo fmt) to fix spacing and
ordering and ensure the use statements (e.g., CoreBackendMode, SpvStatus,
Component, ConfirmationDialog/ConfirmationStatus, add_left_panel,
island_central_panel, add_top_panel, WalletUnlockPopup/WalletUnlockResult,
copy_text_to_clipboard, DashColors,
AccountCategory/AccountSummary/collect_account_summaries,
MessageType/RootScreenType/ScreenLike/ScreenType, DateTime/Utc,
spv_phase_summary, Address) are properly grouped and sorted per project style so
CI formatting checks pass.
src/context/connection_status.rs (1)

333-347: ⚠️ Potential issue | 🟡 Minor

Refresh-throttle comment is outdated.

Line 333 says “once every 2 seconds,” but current behavior is 4s (synced), 1s (non-synced), and 200ms (SPV stopping).

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/context/connection_status.rs` around lines 333 - 347, The comment above
the refresh-throttle logic is inaccurate; update the comment to reflect the
actual timeout behavior used by the code in the block that reads
self.last_update, calls Instant::now(), checks self.spv_status() ==
SpvStatus::Stopping and self.overall_state() == OverallConnectionState::Synced,
and sets timeout to Duration::from_millis(200) for stopping, REFRESH_CONNECTED
(4s) when synced, and REFRESH_DISCONNECTED (1s) otherwise; ensure the comment
references these three concrete durations and the conditions
(SpvStatus::Stopping, OverallConnectionState::Synced) so it stays correct.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@docs/ai-design/2026-02-24-sync-status-panel/manual-test-scenarios.md`:
- Around line 121-122: Update the described SPV expected outputs to match the
new phase-summary formatter: replace occurrences of the old "Core: Syncing --
Headers NN%" pattern with the new format that shows "current / target (pct%)"
(e.g., "Core: Syncing -- Headers 1234 / 5678 (21%)"), and add coverage for the
new "Masternodes" phase where applicable; update the related entries that still
assume only "NN%" (including the other noted occurrences of the same text) so
all examples and test scenarios show the full "current / target (pct%)" form and
include Masternodes phase output where the app now reports it.

In `@src/backend_task/core/create_asset_lock.rs`:
- Around line 53-57: The new tracing::warn! calls are not rustfmt-compliant;
locate the two uses of tracing::warn! around the
transactions_waiting_for_finality.lock() error paths (the ones that warn "Failed
to clean up finality tracking for tx {tx_id}: Mutex poisoned") and reformat the
macro invocation so the string and arguments are wrapped across lines per
rustfmt (e.g., put the format string on its own line and the variables/arguments
on following indented lines or use a single formatted string via format! before
calling tracing::warn!). Ensure both occurrences use identical, rustfmt-friendly
wrapping and compile without changing the logged message or surrounding logic.

---

Outside diff comments:
In `@src/context/connection_status.rs`:
- Around line 333-347: The comment above the refresh-throttle logic is
inaccurate; update the comment to reflect the actual timeout behavior used by
the code in the block that reads self.last_update, calls Instant::now(), checks
self.spv_status() == SpvStatus::Stopping and self.overall_state() ==
OverallConnectionState::Synced, and sets timeout to Duration::from_millis(200)
for stopping, REFRESH_CONNECTED (4s) when synced, and REFRESH_DISCONNECTED (1s)
otherwise; ensure the comment references these three concrete durations and the
conditions (SpvStatus::Stopping, OverallConnectionState::Synced) so it stays
correct.

In `@src/ui/wallets/wallets_screen/mod.rs`:
- Around line 2140-2156: The PlatformAddressBalances branch updates the selected
wallet but always calls self.refresh_platform_sync_info_cache(&seed_hash) even
if the result no longer corresponds to the currently selected wallet; modify the
BackendTaskSuccessResult::PlatformAddressBalances handling so that after
verifying the selected wallet (the selected_wallet read lock and
wallet.seed_hash() == seed_hash) and updating balances via
wallet.set_platform_address_info(...) you only call
self.refresh_platform_sync_info_cache(&seed_hash) when that same selected wallet
check succeeded (i.e., wrap the refresh call inside the same if-let guard or
re-check selected_wallet and seed_hash before calling
refresh_platform_sync_info_cache).
- Around line 12-27: The import block in this module is misformatted; run
rustfmt (cargo fmt) to fix spacing and ordering and ensure the use statements
(e.g., CoreBackendMode, SpvStatus, Component,
ConfirmationDialog/ConfirmationStatus, add_left_panel, island_central_panel,
add_top_panel, WalletUnlockPopup/WalletUnlockResult, copy_text_to_clipboard,
DashColors, AccountCategory/AccountSummary/collect_account_summaries,
MessageType/RootScreenType/ScreenLike/ScreenType, DateTime/Utc,
spv_phase_summary, Address) are properly grouped and sorted per project style so
CI formatting checks pass.

ℹ️ Review info

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0406d47 and cb8ceaf.

📒 Files selected for processing (5)
  • docs/ai-design/2026-02-24-sync-status-panel/manual-test-scenarios.md
  • src/backend_task/core/create_asset_lock.rs
  • src/context/connection_status.rs
  • src/ui/components/top_panel.rs
  • src/ui/wallets/wallets_screen/mod.rs

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 7 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

- Update test scenarios TS-07 through TS-11 and TS-23 to reflect new
  SPV phase format: "Headers: C / T (NN%)" instead of "Headers NN%"
- Add Masternodes phase to TS-23 progression
- Add developer mode precondition to test scenarios
- Fix tooltip showing "syncing..." when SPV is fully synced (Running)
- Update stale throttle comment to reflect new refresh rates

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@lklimek lklimek enabled auto-merge (squash) February 25, 2026 11:54
@lklimek lklimek disabled auto-merge February 25, 2026 12:12
@lklimek lklimek merged commit 260d18d into v1.0-dev Feb 25, 2026
4 checks passed
@lklimek lklimek deleted the zk-extract/sync-status-panel branch February 25, 2026 12:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants